refactor: Build user input prompts with intent constructors - #459
Conversation
8ea1c73 to
d063210
Compare
| m.sink.Emit(output.UserInputRequestEvent{ | ||
| Prompt: "LocalStack is still starting. Check progress with 'lstk logs'.", | ||
| Options: []output.InputOption{ | ||
| {Key: "w", Label: "[W] Keep waiting"}, | ||
| {Key: "s", Label: "[S] Stop and exit"}, | ||
| m.sink.Emit(output.ActionChoice( | ||
| "LocalStack is still starting. Check progress with 'lstk logs'.", | ||
| []output.InputOption{ | ||
| {Key: "w", Label: "Keep waiting"}, | ||
| {Key: "s", Label: "Stop and exit"}, | ||
| }, | ||
| ResponseCh: responseCh, | ||
| Vertical: true, | ||
| }) |
There was a problem hiding this comment.
This shows well the key benefits of the change.
- name talks about a specific intent
- rendering details that are easy to miss (
Vertical) are hidden behindActionChoiceimplementation. - Key rendering like
[W]is derived from the key itself, doesn't leave a place for drift
| Prompt: "Reset emulator state? All resources will be lost", | ||
| Options: []output.InputOption{ | ||
| {Key: "y", Label: "Yes"}, | ||
| {Key: "n", Label: "NO"}, |
There was a problem hiding this comment.
Was inconsistent with the rest of y/n prompts: Yes/NO here, y/N everywhere else.
| responseCh := make(chan output.InputResponse, 1) | ||
| sink.Emit(output.UserInputRequestEvent{ | ||
| Prompt: "Which emulator would you like to use?", | ||
| Options: options, |
There was a problem hiding this comment.
Actual shortcut keys to select the emulators were not displayed because ShortName() didn't contain the text:
It's hard to understand while writing the code because InputOption receives both key and label, so agents often assume label doesn't need to contain the key shortcut prefix in their text.
With the refactoring the rendering is left to the ActionChoice implementation and is consistent between usages - shortcut keys are always displayed if provided. prevents the problem and keeps ShortName() clear and without a possibility to drift from defined shortcut key to what is typed in the label
| responseCh := make(chan output.InputResponse, 1) | ||
| sink.Emit(output.UserInputRequestEvent{ | ||
| Prompt: "Update lstk to latest version?", | ||
| Options: []output.InputOption{{Key: "u", Label: "Update now [U]"}, {Key: "r", Label: "Remind me next time [R]"}, {Key: "s", Label: "Skip this version [S]"}}, |
There was a problem hiding this comment.
Here key shortcut was printed as a suffix (Update now [U]), while in every other user prompt it is a prefix (e.g. [R] Re-authenticate).
Leaving rendering the shortcuts to ActionChoice implementation rather than typing it out in a label string keeps the formatting consistent across CLI.
| Options []InputOption | ||
| ResponseCh chan<- InputResponse | ||
| Vertical bool | ||
| prompt string |
There was a problem hiding this comment.
Keeping fields package-private by using lowercase names. This means UserInputRequestEvent can't be meaningfully instantiated directly. prompt.go is in the same package so new constructors can set these fields when they build an instance.
| }) | ||
| sink.Emit(output.Confirm( | ||
| fmt.Sprintf("Delete cloud snapshot 'pod:%s'? This operation cannot be undone.", podName), | ||
| output.DefaultNo, |
There was a problem hiding this comment.
There is a behaviour change here - the default is changed to No. Rationale - other destructive operations, e.g. reset emulator, clear volume, have No as a default - meaning that accidentally pressing Enter does not perform an operation that cannot be undone. I've made snapshot remove consistent with the rest here - @anisaoshafi would love to hear your opinion since you worked on snapshots recently IIRC.
| vertical bool | ||
| } | ||
|
|
||
| func (e UserInputRequestEvent) Prompt() string { return e.prompt } |
There was a problem hiding this comment.
Adding accessors for fields that are package-private now - they need to be read by rendering code in other packages, e.g. in app.go.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
f07e12d to
49745bc
Compare
gtsiolis
left a comment
There was a problem hiding this comment.
Clean refactor moving UserInputRequestEvent construction behind three intent constructors (Confirm/ActionChoice/Acknowledge) with private fields, migrating every call site and its tests, and centralizing shortcut derivation in OptionLabel.
- thought(non-blocking): on
internal/snapshot/remove.go— this quietly changes user-facing behavior: the old delete prompt used labels{Y, n}, so ENTER confirmed the delete, whereasConfirm(DefaultNo)makes ENTER cancel it. That's strictly safer for an irreversible op and is covered by the new[y/N]assertion, but a behavior change riding inside a "refactor" PR is easy to miss — worth a line in the description so reviewers know it's intentional. (Same applies toreset/volume clear, though those already defaulted to No.) - praise: the coverage migration is careful — the app-level tests that could only be built from hand-rolled option shapes are removed, and their rules (explicit-
enterpriority over an uppercase default, non-letter labels,any-key priority) are already exercised directly againstresolveOptioninTestResolveOption, so nothing is actually lost.
Automated review on behalf of @gtsiolis.
Generated by Claude Code
👍 This change was already mentioned in the self-review: #459 (comment). But I agree it is worth highlighting in the PR description. Added. |
Motivation
#457 and #458 each fixed one prompt that had shipped with the wrong layout, and the review comment on #457 wrote down the rule they were both applying:
That rule is not written anywhere.
UserInputRequestEventasked its author for aVertical bool, which is a rendering decision. It is easy to leave the field out which defaults to inline.Also, labels had drifted into three styles at once:
[ENTER] Log in again,Update now [U], and a bareAWSadvertising no key at all.Solution
Give user input a vocabulary of intents and derive the layout from it. User promts can be built using one of the new constructors in
internal/output/prompt.go. The developer uses the appropriate prompt type which automatically applies the design rule :output.Confirm(prompt, DefaultYes|DefaultNo, ch)[y/N], capitalized answer is what ENTER picksoutput.ActionChoice(prompt, options, ch)output.Acknowledge(prompt, label, ch)UserInputRequestEventcan't be used directly, but can be easily extended with a new prompt type if needed. See technical details:Technical details
To enforce the usage of constructors,
UserInputRequestEvent's fields are unexported - this is done by renaming variables from uppercase to lowercase and read-only accessors are added. This way a struct literal built anywhere outsideinternal/outputdoes not compile:Unexporting only
Verticalwould not have worked — Go lets a keyed literal from another package omit unexported fields, so the bypass would still compile and would still default to the inline layout that caused the bug. Removing every exported field is what closes it.UserInputRequestEvent{}remains legal but inert: no prompt, nil channel, nothing settable.Behavior changes
The default has been changed to No for
lstk snapshot remove. Rationale - other destructive operations, e.g.lstk reset,lstk volume clear, have No as a default - meaning that accidentally pressing Enter does not perform an operation that cannot be undone. I've made snapshot remove consistent with the rest. See comments in review below: #459 (comment)Review
I've reviewed the code and tested the behaviour manually. I've also shortened generated comments and claude instructions.
I've added a self-review with examples of inconsistencies that this PR fixes and prevents in the future.
Validation
go test ./internal/... ./cmd/... -count=1golangci-lint run ./...— 0 issuesmake buildinternal/reset/reset.goand confirminggo build ./...rejects itFollow-up to #457 and #458.
Closes DEVX-1057